Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handling of interruptions #503

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Handling of interruptions #503

wants to merge 9 commits into from

Conversation

JulienDoerner
Copy link
Member

Dear all,

This PR introduces a new handling of interruption of CRPropa simulations. The idea is to store all candidates which are currently propagating in the simulation and write them into an output file. Additionally the information about the number of not yet started candidates will be displayed.
Having this information it is possible to continue the simulation after it was interrupted.

handling

To use the storing at an interruption the user has to provide an output module to the moduleList.

 sim = ModuleList() 
 ... 
 
 output_on_break = TextOutput(my_file.txt)
 sim.setInterruptAction(output_on_break)

After the break the candidates which have stored can be reloaded with the particleCollector.
In case of a simulation with a Candidate Vector the output file will also contain the indices of the not started particles.
This allows a real tracing that all particles are covered and nothing is lost.

examples in the documentation

To illustrate this new feature two example notebooks (one with a source interface and one with a candidate vector) are added to the documentation.

The example with the source interface needs some manual adaption of the number of not started Candidates.
And both example need a manually triggered Keyboard Interrupt.
Therefore, both notebooks are excluded from the example notebook testing.

rafaelab
rafaelab requested changes Sep 7, 2024
View reviewed changes
Copy link
Member

@rafaelab rafaelab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting PR, @JulienDoerner. It seems to be working fine.

I left a few style-related comments.

However, one thing I find a bit problematic is the fact that you are calling a module (module/Output.h) from inside the ModuleList.
Avoiding this type of cross talks and have completely independent module is, for better or for worse, the spirit of CRPropa (although in this case I personally don't see how things could go wrong).

Therefore, my question is: can't this implementation be moved around to be triggered in the observer, like what is done for onDetection? Maybe there could be an onInterruption method that does the same thing?
This would avoid interactions between modules and hold for all types of outputs, I suppose.

src/ModuleList.cpp.in Outdated Show resolved Hide resolved
src/ModuleList.cpp.in Outdated
if (cand -> isActive())
interruptAction -> process(cand);
else
KISS_LOG_WARNING << "Try to dump a candidate which is not active anymore! \n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to improve the message. It contains the function which is raising the error and the the serial number of the candidate to trace down the reason for this.

src/ModuleList.cpp.in Outdated Show resolved Hide resolved
src/ModuleList.cpp.in Outdated Show resolved Hide resolved
src/ModuleList.cpp.in Outdated Show resolved Hide resolved
@JulienDoerner
Copy link
Member Author

@rafaelab

I applied your code style comments.

About the modules:
The interruptAction is an independent module like everywhere else in the code. This design is designed like the onDetection in the observer, where an Output module is given to the observer and it will be processed on the detection. The same is working for the interruption.
I think we could rename it to onInterruption for more consistency.

I would not suggest to put it into the observer (which is stored inside the module list), as it could lead to some confusions if you have different observers in the same simulation.

@rafaelab
Copy link
Member

I would not suggest to put it into the observer (which is stored inside the module list), as it could lead to some confusions if you have different observers in the same simulation.'

Hm... That does seem to make sense.

However, I'm personally very reluctant to see modules in the ModuleList, as it breaks the underlying code logical structure and compromises the clean design. If this change is allowed, the code will no longer be 100% modular.
However, I do no have a better suggestion to achieve this.
What do you think, @lukasmerten ?

Maybe something like this could be though of.
Any time you save a particle, according to the current structure, an Observer is required.
In this case, it would be a special type of observer similar to ObserverDetectAll (e.g., ObserverUnprocessed or ObserverWaitingList or whatever) that dumps the output onInterruption.
For instance:

output = TextOutput()
obsInterrupt = ObserverUnprocessed()
obsInterrupt.onInterrupt(output)

@lukasmerten
Copy link
Member

Hi @rafaelab and @JulienDoerner
I have to think about this but won't have time until next week. I'll let you know if I come up with something useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelab rafaelab rafaelab requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JulienDoerner @rafaelab @lukasmerten